[WIP] OTA-1975: add products-data container and service to service products.json if present in graph-data image#270
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ankitathomas: This pull request references OTA-1975 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughAdds a products-service container and ClusterIP Service, configures its deployment resources and health probes, reconciles the Service, and prepares product lifecycle data in the development image. ChangesProducts data service
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant UpdateServiceReconciler
participant KubernetesService
participant products-service
participant products.json
UpdateServiceReconciler->>KubernetesService: ensure products-data Service
KubernetesService->>products-service: route port 8082 traffic
products-service->>products.json: read products data
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ankitathomas The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…resent in graph-data image Signed-off-by: Ankita Thomas <ankithom@redhat.com>
ankitathomas
force-pushed
the
products-data
branch
from
8e9a48e to
12324be
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
controllers/new.go (1)
1040-1133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider avoiding repeated magic port literal and hardcoded bind address.
8082is repeated 5 times across Args/Ports/Probes/Service, and"::"is hardcoded rather than sourced from a ConfigMap/env likeADDRESSis for policy-engine. Extracting a shared constant for the port would reduce drift risk, and aligning the address with the existing env-config pattern would improve consistency with the rest of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/new.go` around lines 1040 - 1133, Update newProductsServiceContainer and newProductsService to use a shared named constant for port 8082 across arguments, container/service ports, and probes, and replace the hardcoded "::" bind address with the file’s established ConfigMap or environment-based ADDRESS pattern used by policy-engine.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/new.go`:
- Line 1063: Update newProductsServiceContainer to use dedicated minimal volume
mounts instead of newGraphBuilderVolumeMounts(instance). Mount only the
cincinnati-graph-data volume at the products graph-data path needed to serve
products.json, excluding /etc/configs, pull-secret, and trusted-ca; then update
the affected deployment fixture to match.
In `@dev/Dockerfile`:
- Around line 7-10: Update the products.json setup commands so curl treats
non-2xx responses as failures and uses the existing empty-object fallback, then
remove the unconditional `|| true` from the mkdir/mv command. Ensure directory
creation and moving products.json fail the image build when placement is
unsuccessful.
---
Nitpick comments:
In `@controllers/new.go`:
- Around line 1040-1133: Update newProductsServiceContainer and
newProductsService to use a shared named constant for port 8082 across
arguments, container/service ports, and probes, and replace the hardcoded "::"
bind address with the file’s established ConfigMap or environment-based ADDRESS
pattern used by policy-engine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3700c0d7-8643-43c6-b35b-5beb34cbac30
📒 Files selected for processing (5)
controllers/names.gocontrollers/new.gocontrollers/testdata/resources/zz_fixture_Test_newKubeResources_deployment.yamlcontrollers/updateservice_controller.godev/Dockerfile
| Protocol: corev1.ProtocolTCP, | ||
| }, | ||
| }, | ||
| VolumeMounts: k.newGraphBuilderVolumeMounts(instance), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Products container is over-mounted with unrelated secrets/config.
newProductsServiceContainer reuses k.newGraphBuilderVolumeMounts(instance) wholesale, which (per the fixture) mounts /etc/configs (gb.toml), pull-secret (registry credentials), and trusted-ca in addition to cincinnati-graph-data. The products-service only needs read access to /var/lib/cincinnati/graph-data/products to serve products.json; there's no evident need for registry pull-secret or trusted CA material in this container. This unnecessarily widens the container's access to sensitive data (registry credentials) for a component whose only job is serving a static JSON file.
🔒 Proposed fix: dedicated, minimal volume mounts
- VolumeMounts: k.newGraphBuilderVolumeMounts(instance),
+ VolumeMounts: []corev1.VolumeMount{
+ {
+ MountPath: "/var/lib/cincinnati/graph-data",
+ Name: "cincinnati-graph-data",
+ },
+ },Note: the fixture file controllers/testdata/resources/zz_fixture_Test_newKubeResources_deployment.yaml will need updating to match once this is fixed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| VolumeMounts: k.newGraphBuilderVolumeMounts(instance), | |
| VolumeMounts: []corev1.VolumeMount{ | |
| { | |
| MountPath: "/var/lib/cincinnati/graph-data", | |
| Name: "cincinnati-graph-data", | |
| }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/new.go` at line 1063, Update newProductsServiceContainer to use
dedicated minimal volume mounts instead of
newGraphBuilderVolumeMounts(instance). Mount only the cincinnati-graph-data
volume at the products graph-data path needed to serve products.json, excluding
/etc/configs, pull-secret, and trusted-ca; then update the affected deployment
fixture to match.
| RUN curl -L -o products.json https://access.redhat.com/product-life-cycles/api/v2/products || echo "{}" > products.json | ||
|
|
||
| RUN mkdir -p /var/lib/cincinnati-graph-data/products && mv products.json /var/lib/cincinnati-graph-data/products/ || true | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Curl/mv failures are silently swallowed — build succeeds even when products.json fetch/placement fails.
- Line 7:
curllacks-f/--fail, so a non-2xx HTTP response (e.g. API outage, 404/500) still exits 0 and its body gets written intoproducts.jsoninstead of triggering the{}fallback. - Line 9:
|| trueswallows anymkdir/mvfailure, so the build succeeds even ifproducts.jsonnever ends up under/products/, leavingproducts-servicewithout data at runtime with no build-time signal.
🛠️ Proposed fix
-RUN curl -L -o products.json https://access.redhat.com/product-life-cycles/api/v2/products || echo "{}" > products.json
+RUN curl -f -L -o products.json https://access.redhat.com/product-life-cycles/api/v2/products || echo "{}" > products.json
-RUN mkdir -p /var/lib/cincinnati-graph-data/products && mv products.json /var/lib/cincinnati-graph-data/products/ || true
+RUN mkdir -p /var/lib/cincinnati-graph-data/products && mv products.json /var/lib/cincinnati-graph-data/products/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN curl -L -o products.json https://access.redhat.com/product-life-cycles/api/v2/products || echo "{}" > products.json | |
| RUN mkdir -p /var/lib/cincinnati-graph-data/products && mv products.json /var/lib/cincinnati-graph-data/products/ || true | |
| RUN curl -f -L -o products.json https://access.redhat.com/product-life-cycles/api/v2/products || echo "{}" > products.json | |
| RUN mkdir -p /var/lib/cincinnati-graph-data/products && mv products.json /var/lib/cincinnati-graph-data/products/ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dev/Dockerfile` around lines 7 - 10, Update the products.json setup commands
so curl treats non-2xx responses as failures and uses the existing empty-object
fallback, then remove the unconditional `|| true` from the mkdir/mv command.
Ensure directory creation and moving products.json fail the image build when
placement is unsuccessful.
|
@ankitathomas: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary by CodeRabbit